Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build(ui): improve performance with esbuild-loader #12516

Merged
merged 5 commits into from
May 13, 2024

Conversation

agilgur5
Copy link

@agilgur5 agilgur5 commented Jan 14, 2024

Follow-up to #11628

Motivation

  • similar to Argo CD and many other projects, we can use esbuild instead of TS etc to significantly speed up build times
    • (as esbuild is written in Go and compiled instead of using interpreted TS/JS)
    • we don't use any Babel or TS plugins, so I think this codebase is a good candidate for that
    • it also simplifies the build process as well, removing & replacing many other deps
    • caveats:
  • resolve various warnings in the build, during the install, and in the console
  • replace multiple deps with 1

Modifications

  • remove babel-loader, ts-loader, react-hot-loader, and source-map-loader
    • note that source-map-loader is still present in the yarn.lock as it is used under-the-hood by other deps
      • but the warnings from directly using source-map-loader are no longer present in the build
      • and the bundle size shrank a bit, seemingly bc there is no autolinker etc dep in the bundle, but potentially other reasons (I did not check in-depth)

Related config / performance changes

Verification

  1. yarn build works fine
  2. make start UI=true works fine and UI seems to function as expected

Performance Testing

some simple performance testing when running natively on macOS, taken directly from Webpack's reported stats:

  • before:
    • prod build: ~65s
    • dev build: ~22s
    • dev rebuild: ~1.8s
  • after:
    • prod build: ~50s (~25% faster)
    • dev build: ~16s (~25% faster)
    • dev rebuild: ~.8s (~50% faster)

Future Work

these are not as good improvements as I would have liked, which mean there are bigger bottlenecks elsewhere

@agilgur5 agilgur5 added type/dependencies PRs and issues specific to updating dependencies area/ui area/build Build or GithubAction/CI issues javascript Pull requests that update Javascript dependencies labels Jan 14, 2024
@agilgur5
Copy link
Author

Gah, this already got a merge conflict with a Dependabot PR #12511. Adding prioritized-review label to this and #12487 as a result

@agilgur5 agilgur5 force-pushed the build-ui-esbuild-loader branch from 70c68f6 to 879bdfa Compare January 14, 2024 02:01
@agilgur5
Copy link
Author

Oh, interesting, due to the new isolatedModules config, the build is failing due to missing import type. i.e. this PR actually depends on #12514

@agilgur5 agilgur5 requested a review from isubasinghe January 16, 2024 01:16
Copy link
Member

@isubasinghe isubasinghe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, this is so much faster.
Works as expected

@terrytangyuan terrytangyuan enabled auto-merge (squash) January 18, 2024 10:26
@terrytangyuan
Copy link
Member

Build failed

@isubasinghe
Copy link
Member

@terrytangyuan you need to merge this one first: #12514

@agilgur5 agilgur5 added the prioritized-review For members of the Sustainability Effort label Jan 22, 2024
auto-merge was automatically disabled January 22, 2024 17:25

Head branch was pushed to by a user without write access

@agilgur5 agilgur5 force-pushed the build-ui-esbuild-loader branch from 879bdfa to 9d49b09 Compare January 22, 2024 17:25
@agilgur5
Copy link
Author

Fixed another merge conflict in the yarn.lock. Still waiting for #12514 to be merged so that this can be unblocked and merged

@juliev0
Copy link
Contributor

juliev0 commented Feb 3, 2024

@isubasinghe are you comfortable being the Assignee for this one, so you may be able to merge it once you officially have access to?

@agilgur5
Copy link
Author

agilgur5 commented Feb 3, 2024

Merged main since #12514 was merged, but getting a test failure in argo-ui now 😕 . Interestingly enough, the build passes fine (which I had tested), it's only the tests' build that's failing, I think because the tooling is different (ts-jest uses the TS compiler directly, esbuild-loader uses esbuild)?

I was trying to partly workaround a CSS side-effect import in #12158 for similar reasons to this, but otherwise this requires a few changes upstream in argo-ui (where my PRs have been open even longer 😕)

I guess I'll kick this back to draft as a result since it is pretty blocked on argo-ui -- but uh may want my argo-ui PRs to be prioritized 😅 (the label does not exist there ofc)

@agilgur5 agilgur5 removed the prioritized-review For members of the Sustainability Effort label Feb 3, 2024
@agilgur5 agilgur5 marked this pull request as draft February 3, 2024 18:51
@juliev0
Copy link
Contributor

juliev0 commented Feb 3, 2024

I guess I'll kick this back to draft as a result since it is pretty blocked on argo-ui -- but uh may want my argo-ui PRs to be prioritized 😅 (the label does not exist there ofc)

Okay, yes!

@isubasinghe isubasinghe self-assigned this Feb 3, 2024
@isubasinghe
Copy link
Member

@isubasinghe are you comfortable being the Assignee for this one, so you may be able to merge it once you officially have access to?

Yup happy to merge when the tests pass

@agilgur5
Copy link
Author

agilgur5 commented May 11, 2024

I guess I'll kick this back to draft as a result since it is pretty blocked on argo-ui -- but uh may want my argo-ui PRs to be prioritized 😅 (the label does not exist there ofc)

I might be able to get this working again now that argoproj/argo-ui#509 and argoproj/argo-ui#552 have been merged

- similar to [Argo CD](https://github.com/argoproj/argo-cd/blob/d9df2525c57d4870215a6ce149dbedd08ae05fdb/ui/src/app/webpack.config.js#L37) and many other projects, we can use `esbuild` instead of TS etc to significantly speed up build times
  - (as `esbuild` is written in Go and compiled instead of using interpreted TS/JS)
  - we don't use any Babel or TS plugins, so I think this codebase is a good candidate for that
  - it also simplifies the build process as well, removing & replacing many other deps
  - caveats:
    - `esbuild` does not do type-checking, so we have to run `tsc --noEmit` as part of the `lint` phase
      - `isolatedModules` config is needed to ensure compatibility with this
    - `esbuild` does not currently support React Fast Refresh
      - `react-hot-loader` was replaced by [React Fast Refresh](https://github.com/pmmmwh/react-refresh-webpack-plugin) and is deprecated / hasn't received an update in 2 years
        - remove `module.hot` etc usage of `react-hot-loader` as well
      - that being said, `esbuild` is significantly faster than using Babel or TS for builds, so hot reloading is nice but not necessary
        - also hot reloading can be buggy

- further optimize dev builds by using cheaper `eval` sourcemaps as [recommended by Webpack for performance](https://webpack.js.org/configuration/devtool/)
  - prod builds should always have highest quality sourcemaps, so do not change those

- remove unused `react-paginate` `exclude`
  - it's not a dep we currently use; it doesn't exist in the lockfile
    - (nor in `argo-ui`'s lockfile, of which all prod deps would be in Workflows's lockfile)

some simple performance testing when running natively on macOS, taken directly from Webpack's reported stats:
- before:
  - prod build: ~65s
  - dev build: ~22s
  - dev rebuild: ~1.8s
- after:
  - prod build: ~50s (~25% faster)
  - dev build: ~16s (~25% faster)
  - dev rebuild: ~.8s (~50% faster)
- these are not as good improvements as I would have liked, which mean there are bigger bottlenecks elsewhere
  - e.g. Webpack itself (analysis, source maps, running loaders), sass-loader, etc
  - switching to `esbuild` directly may further improve these stats
    - i.e. instead of Webpack + `esbuild-loader`

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5 agilgur5 force-pushed the build-ui-esbuild-loader branch from 147fff6 to f52df81 Compare May 11, 2024 16:56
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5
Copy link
Author

Guh, this is now failing to typecheck due to another dep, this time very transitive: ramda/ramda#3415 (comment), swagger-api/swagger-js#3176, swagger-api/apidom#3279

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5
Copy link
Author

Ok had to fix that by pinning types-ramda as it requires TS v5 per ramda/ramda#3415 (comment) and swagger-api/swagger-js#3176 (comment)

Anton Gilgur added 2 commits May 11, 2024 13:34
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@agilgur5
Copy link
Author

Verification

  1. yarn build works fine
  2. make start UI=true works fine and UI seems to function as expected

Re-verified, seems to all be fine

@agilgur5 agilgur5 marked this pull request as ready for review May 11, 2024 19:02
@agilgur5 agilgur5 linked an issue May 13, 2024 that may be closed by this pull request
4 tasks
@agilgur5
Copy link
Author

I'm gonna go ahead and merge this since I only made the changes necessary to fix CI since this was last approved. It also fixes a bunch of warnings and improves performance etc, so it's pretty darn nice to have sooner rather than later

@agilgur5 agilgur5 merged commit cd52436 into argoproj:main May 13, 2024
18 checks passed
@agilgur5 agilgur5 deleted the build-ui-esbuild-loader branch May 13, 2024 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build Build or GithubAction/CI issues area/ui javascript Pull requests that update Javascript dependencies type/dependencies PRs and issues specific to updating dependencies type/tech-debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dev UI shows popup with warnings
4 participants